-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix opacity adjustment and set it to low non-zero value #280
Conversation
The changes are minimal, I added both @stefanhahmann and @thorstenwagner as reviewers because you are somehow involved in the issues. Let me know if you agree with the changes. |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #280 +/- ##
=======================================
Coverage 76.52% 76.52%
=======================================
Files 16 16
Lines 1853 1853
=======================================
Hits 1418 1418
Misses 435 435 ☔ View full report in Codecov by Sentry. |
I would generally agree to the changes but it would be good to hear from the others! Apart from that feel free to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -124,7 +124,7 @@ def manual_clustering_method(inside): | |||
plot_cluster_name=clustering_ID, | |||
) | |||
if isinstance(self.analysed_layer, Labels): | |||
self.layer_select.value.opacity = 0 | |||
self.layer_select.opacity = 0.2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested a bit. I would prefer 0.25 or even 0.3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to not increase it too much so maybe we can stick to 0.25, so that the experience does not change too dramatically?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
0.25 would be fine for. (I could also live with 0.2). The slight increase was just a subjective suggestion.
@zoccoler, |
I just tested this a bit on my machine and it did not seem to make a difference, even though I would agree, that one might expect a difference. |
Thanks for checking this so soon! I am fine with merging this from my end maybe @thorstenwagner can chime in over the next few days if he is also fine with this? otherwise I'll merge it. |
I totally forgot to check that. Will do it today. |
Hi, from my side everything still works as it should be. Probably because I set the opacity to zero when I load the the plugin: |
Perfect! Then I'll merge this fixing #279 |
This should fix #279 and could potentially solve #271 .
This lets the selected layer barely visible (
opacity = 0.2
), which would not lead (I think) to the user thinking the original labels layer was broken, but still hide it quite a bit so that the clustering result does not blend much with the labels colors.The best option in my opinion is to interactively hide and show it, which is already written in the documentation under Manual clustering.
Diving deep into git blame, it seems this was set by @thorstenwagner here, which then was later updated to setting opacity to
0
. @thorstenwagner, would it be OK for your use cases to let a small opacity like0.2
?Best,
Marcelo